-
Notifications
You must be signed in to change notification settings - Fork 372
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Run tests using a container #2704
Conversation
value: '8e037ad4-618f-4466-8bc8-5099d41ac15b' | ||
- name: testsSourcesDirectory | ||
value: "$(Build.SourcesDirectory)/tests_e2e" | ||
value: 'azuremanagement' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed the connection and made the actual subscription ID a parameter.
@@ -0,0 +1,74 @@ | |||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is basically the previous install_dependencies.sh now as a Dockerfile
@@ -10,4 +10,3 @@ cryptography | |||
distro | |||
junitparser | |||
msrestazure | |||
python-dotenv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this dependency. We are over-using the environment to pass data across different components. We need to reduce the use of environment variables, so this module should not be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree but at the same time it's going to be challenging if the data is scope to that component is needed by other component like resource group and names if any task has hard dependency on it. We need to figure out way to pass between components otherwise we have to live with env.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use of environment variables should be restricted to the interface with the Azure pipelines and probably a few (if any) places. If used for other things other than communication with the pipeline, we need to be very explicit about what variables we are using. There should not be a need for this module to save and restore the entire environment.
@@ -12,8 +12,8 @@ | |||
from azure.mgmt.resource import ResourceManagementClient | |||
from msrestazure.azure_exceptions import CloudError | |||
|
|||
from dcr.scenario_utils.logging_utils import LoggingHandler | |||
from dcr.scenario_utils.models import get_vm_data_from_env, VMModelType, VMMetaData | |||
from tests_e2e.scenario_utils.logging_utils import LoggingHandler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixing some dependencies I missed in my previous PR
Codecov Report
@@ Coverage Diff @@
## develop #2704 +/- ##
========================================
Coverage 71.95% 71.95%
========================================
Files 104 104
Lines 15765 15765
Branches 2244 2244
========================================
Hits 11343 11343
Misses 3906 3906
Partials 516 516 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@@ -0,0 +1,33 @@ | |||
# Create a base class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I fixed the dependencies, I realized I missed this file in my previous PR. This file comes from the current Azure Pipeline and needs to be rewritten
@@ -1,43 +0,0 @@ | |||
#!/usr/bin/env bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Dockerfile replaces this script
tests_e2e/docker/Dockerfile
Outdated
cd lisa && \ | ||
poetry config virtualenvs.in-project true && \ | ||
make setup && \ | ||
chmod +r /home/waagent/.config/pypoetry/config.toml && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this chmod is a left-over from some experimentation I was doing, i'll remove it
sudo chown 1000 "$HOME/logs" | ||
|
||
docker run --rm \ | ||
--volume "$BUILD_SOURCESDIRECTORY:/home/waagent/WALinuxAgent" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"--volume <host_path>:<container_path>" shares the local host_path with the container as container-path.
we are sharing the agent's code and the the ssh key used to connect to the test VMs (both of which Azure Pipelines downloaded to the VM); also, we are specifying a location where the container will drop the logs and then the host VM will pick them up from there to report results.
@@ -10,4 +10,3 @@ cryptography | |||
distro | |||
junitparser | |||
msrestazure | |||
python-dotenv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree but at the same time it's going to be challenging if the data is scope to that component is needed by other component like resource group and names if any task has hard dependency on it. We need to figure out way to pass between components otherwise we have to live with env.
--log_path "$HOME/logs" \ | ||
--working_path "$HOME/logs" \ | ||
-v subscription_id:"$SUBSCRIPTION_ID" \ | ||
-v identity_file:"$HOME/.ssh/id_rsa.pub" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The path $HOME refers to container path (/home/waagent), right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this script is executed within the container using the 'waagent' user. $USER, $HOME, etc refer to 'waagent'.
|
||
docker run --rm \ | ||
--volume "$BUILD_SOURCESDIRECTORY:/home/waagent/WALinuxAgent" \ | ||
--volume "$DOWNLOADSSHKEY_SECUREFILEPATH:/home/waagent/id_rsa" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if it's possible, why don't we directly provide /.ssh folder in the container path that way we could avoid doing extra steps copying from one place to other when lisa script run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$BUILD_SOURCESDIRECTORY and $DOWNLOADSSHKEY_SECUREFILEPATH are inputs to the container, while $HOME/logs is its output.
We should not modify the input directories, since the may be coming from a developer's machine while debugging, writing a test, etc (the script that executes the tests creates a file under '/home/waagent' and changes the mode of '/home/waagent/id_rsa')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point. If the file semantics changes in container path would change the input path too. Makes sense.
|
||
docker run --rm \ | ||
--volume "$BUILD_SOURCESDIRECTORY:/home/waagent/WALinuxAgent" \ | ||
--volume "$DOWNLOADSSHKEY_SECUREFILEPATH:/home/waagent/id_rsa" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point. If the file semantics changes in container path would change the input path too. Makes sense.
Installing LISA typically takes 5-7 min. These changes execute the tests using a container where LISA is pre-installed; pulling the container image typically takes 50-90 seconds.
I also fixed some Python imports that I missed in my previous PR, renamed some resources in our pipeline and made some of those resources parameters to the pipeline.